fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872

dpitcock · 2024-10-15T13:06:16Z

Description

Overview

this hides the navigation toggle button when toolbar variant of AppLayout has hideNavigation prop of true.
it also insures there is never a situation where a toolbar of AppLayout will exist with navigationOpen is true when navigationHide is true which is the case in some consoles. This insures the rendering is as expected in those consoles
New page added in separate PR - chore: adding page with reproducible problem #2898 and problem can be reproduced at its deployment https://d21d5uik3ws71m.cloudfront.net/components/b77b2547c6b1210f0de973956d6efb31ae58c212/dev-pages/index.html#/light/app-layout/controlled-navigation/?appLayoutToolbar=true&navigationHide=false&applyNavOpenOverride=true&navOpenOverrideValue=false

Other

renaming mergeProps => mergeMultiAppLayoutProps and SharedProps => SharedMultiAppLayoutProps to be more specific and exporting them so they can be tested individually
testing the mergeMultiAppLayoutProps in isolation on own unit test file
testing the specific case of AppLayout where the navigationHide prop is true with the assertion that no navigation-panel nor navigation toggle is visible.

Related links, issue #, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2024-10-15T13:14:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (f891e72) to head (8ab819e).
Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2872    +/-   ##
========================================
  Coverage   96.21%   96.21%            
========================================
  Files         761      761            
  Lines       21490    21502    +12     
  Branches     7350     6963   -387     
========================================
+ Hits        20676    20688    +12     
- Misses        761      806    +45     
+ Partials       53        8    -45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/app-layout/__tests__/multi-layout.test.tsx

src/app-layout/visual-refresh-toolbar/toolbar/index.tsx

src/app-layout/visual-refresh-toolbar/multi-layout.ts

src/app-layout/visual-refresh-toolbar/toolbar/index.tsx

src/app-layout/visual-refresh-toolbar/index.tsx

…d-navigation page

dpitcock · 2024-10-17T14:11:21Z

src/app-layout/visual-refresh-toolbar/index.tsx

+    //navigation must be null if hidden so toolbar knows to hide the toggle button
+    const resolvedNavigation = navigationHide ? null : navigation ?? <></>;
+    //navigation must not be open if navigationHide is true
+    const resolvedNavigationOpen = !!resolvedNavigation && navigationOpen;


Even if no navigation provided, the fragment passed will be truthy, therefor resolvedNavigation will only be falsy if navigationHide is true

dpitcock · 2024-10-17T16:51:12Z

pages/app-layout/controlled-navigation.page.tsx

+
+export default function () {
+  const {
+    urlParams: { navigationOpen = true, navigationHide = false },


so we can easily set expectations in regression tests

dpitcock · 2024-10-17T16:52:08Z

src/app-layout/visual-refresh-toolbar/multi-layout.ts

@@ -10,7 +10,7 @@ import { AppLayoutProps } from '../interfaces';
 import { Focusable } from '../utils/use-focus-control';
 import { SplitPanelToggleProps, ToolbarProps } from './toolbar';

-interface SharedProps {
+export interface SharedMultiAppLayoutProps {


Only name changes for the interface and function

abdhalees · 2024-10-18T09:04:02Z

src/app-layout/__tests__/multi-layout.test.tsx

+        {...defaultAppLayoutProps}
+        data-testid="first"
+        navigation="testing nav"
+        navigationHide={true}


The second layout using navigationHide is a more common case.

I updated the test name and added some assertions below to cover this case more thoroughly.

…Hide prop

just-boris

small comments only

just-boris · 2024-10-18T13:37:25Z

src/app-layout/__tests__/multi-layout-props.test.tsx

@@ -0,0 +1,109 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+/* eslint-disable simple-import-sort/imports */


We only do this in files which use src/app-layout/__tests__/utils.tsx because it contains side effects.

All other tests can benefit from the linter rule

just-boris · 2024-10-18T13:40:26Z

src/app-layout/__tests__/multi-layout.test.tsx

+    expect(firstLayout.findNavigation()).toBeFalsy();
+    expect(firstLayout.findNavigationToggle()).toBeFalsy();
+    expect(secondLayout.findNavigation()).toBeFalsy();
+    expect(secondLayout.findNavigationToggle()).toBeFalsy();


What do we test here?

Is it about navigationHide is respected, even if navigation is defined? Then we should call it in the test name

dpitcock force-pushed the applayout-controlled-navigation-page branch 2 times, most recently from 3645236 to 3f2d6ef Compare October 16, 2024 17:13

dpitcock commented Oct 16, 2024

View reviewed changes

src/app-layout/__tests__/multi-layout.test.tsx Outdated Show resolved Hide resolved

dpitcock commented Oct 16, 2024

View reviewed changes

src/app-layout/visual-refresh-toolbar/toolbar/index.tsx Outdated Show resolved Hide resolved

dpitcock force-pushed the applayout-controlled-navigation-page branch from 3f2d6ef to b472120 Compare October 16, 2024 17:21

dpitcock commented Oct 16, 2024

View reviewed changes

src/app-layout/visual-refresh-toolbar/multi-layout.ts Outdated Show resolved Hide resolved

dpitcock commented Oct 16, 2024

View reviewed changes

src/app-layout/visual-refresh-toolbar/toolbar/index.tsx Outdated Show resolved Hide resolved

dpitcock deployed to dev-pages October 17, 2024 10:26 — with GitHub Actions View deployment

dpitcock force-pushed the applayout-controlled-navigation-page branch from da33fc5 to 1568590 Compare October 17, 2024 10:35

dpitcock commented Oct 17, 2024

View reviewed changes

src/app-layout/visual-refresh-toolbar/index.tsx Outdated Show resolved Hide resolved

dpitcock deployed to dev-pages October 17, 2024 10:42 — with GitHub Actions View deployment

dpitcock marked this pull request as ready for review October 17, 2024 11:46

dpitcock requested a review from a team as a code owner October 17, 2024 11:46

dpitcock requested review from abdhalees and removed request for a team October 17, 2024 11:46

dpitcock added 3 commits October 17, 2024 16:04

attempt to make page to recreate issue

040c097

fixing/adding unit tests

a586487

seperation of multi-app-layout unit tests, adding params to controlle…

24d2456

…d-navigation page

dpitcock force-pushed the applayout-controlled-navigation-page branch from 1568590 to 24d2456 Compare October 17, 2024 14:05

dpitcock commented Oct 17, 2024

View reviewed changes

dpitcock deployed to dev-pages October 17, 2024 14:13 — with GitHub Actions View deployment

dpitcock requested a review from just-boris October 17, 2024 14:42

dpitcock commented Oct 17, 2024

View reviewed changes

removing controlled-navigation page to add seperately

1febaa2

dpitcock deployed to dev-pages October 18, 2024 08:54 — with GitHub Actions View deployment

abdhalees reviewed Oct 18, 2024

View reviewed changes

asserting no navigation in secondary AppLayout when it has navigation…

c20704f

…Hide prop

dpitcock deployed to dev-pages October 18, 2024 09:35 — with GitHub Actions View deployment

just-boris reviewed Oct 18, 2024

View reviewed changes

small fixes to tests, clearifying test names

8ab819e

dpitcock deployed to dev-pages October 18, 2024 14:02 — with GitHub Actions View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872

fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872

dpitcock commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

dpitcock Oct 17, 2024

dpitcock Oct 17, 2024

dpitcock Oct 17, 2024

abdhalees Oct 18, 2024

dpitcock Oct 18, 2024

just-boris left a comment

just-boris Oct 18, 2024

just-boris Oct 18, 2024

fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872

Are you sure you want to change the base?

fix: hide navigation button when toolbar variant of AppLayout has hideNavigation #2872

Conversation

dpitcock commented Oct 15, 2024 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

dpitcock Oct 17, 2024

Choose a reason for hiding this comment

dpitcock Oct 17, 2024

Choose a reason for hiding this comment

dpitcock Oct 17, 2024

Choose a reason for hiding this comment

abdhalees Oct 18, 2024

Choose a reason for hiding this comment

dpitcock Oct 18, 2024

Choose a reason for hiding this comment

just-boris left a comment

Choose a reason for hiding this comment

just-boris Oct 18, 2024

Choose a reason for hiding this comment

just-boris Oct 18, 2024

Choose a reason for hiding this comment

dpitcock commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading